Skip to content

fix(runner): the MCP guard tested for real, and the MCP routes tested at all (DEV-2501) - #201

Open
danielzytohoc wants to merge 3 commits into
masterfrom
fix/DEV-2501-mcp-route-coverage
Open

fix(runner): the MCP guard tested for real, and the MCP routes tested at all (DEV-2501)#201
danielzytohoc wants to merge 3 commits into
masterfrom
fix/DEV-2501-mcp-route-coverage

Conversation

@danielzytohoc

@danielzytohoc danielzytohoc commented Aug 17, 2026

Copy link
Copy Markdown

Closes the two worst findings of the DEV-2501 coverage audit on the MCP endpoints (POST /api/mcp/demos, PATCH /api/mcp/demos/:id — PRs #170/#177, ADR-0033).

G1 — a security control asserted against a copy of itself

The test "only demos the MCP created are updatable through it" declared its own local predicate:

const fromMcp = (row) => Boolean(row.forked_from?.startsWith("mcp:"));

and asserted against that, not against the route. Deleting the containment guard from index.ts left the test green — the test could not fail when the control it names went away, which is the defining property of a hollow test.

Fixed in three layers:

  • The predicate now lives in production code: isMcpCreated() in workers/api/src/mcp-create.ts, with JSDoc naming it as the containment control from the PR feat(runner): update a demo in place from the MCP (DEV-2501) #177 security review (a leaked shared secret must never reach demos built in the browser).
  • The PATCH /api/mcp/demos/:id route calls it, and the rewritten test imports it (same cases as before: mcp:javascript/mcp:react pass; catalog:javascript, null/undefined, {} refuse).
  • A source-grep test (in the style of pipeline/theme-codegen.test.mjs) asserts the route's source matches /isMcpCreated\(/ and does not match an inline forked_from?.startsWith — so the check cannot silently drift back to a re-inlined copy that diverges from what the tests import.

G2 — no route-level tests at all

Nothing imported the router (workers/api/src/index.ts), so every status code and response shape it promises was untested. New spec pipeline/mcp-routes.test.mjs drives the real default export — request in, Response out — under the existing plain node --test runner.

How it loads: module.register() hooks (pipeline/fixtures/worker-hooks.mjs) map the worker's .js import specifiers onto its .ts sources, and stand a structural stub in for @cloudflare/sandbox, whose cloudflare: scheme imports only exist inside workerd. node --test isolates each spec in its own process, so the hooks never touch other specs. Bindings are in-memory fakes for exactly what the two routes use (D1 with a recorded write log, KV, R2); the create path is steered through createDemo()'s build-cache-hit branch — itself production code — so no test boots a container, and the sandbox stub throws if one is ever requested.

Acceptance criteria now proven at route level:

  • required-description 400, with an assertion that no D1 write and no R2 put happened;
  • 201 create shape: body keys exactly {id, url, embedUrl, editUrl, shareUrl, createdBy} with url === /d/:id etc. — this one looked unreachable (createDemo runs a container build) but the build-cache branch made it testable without any product change;
  • the D1 insert carries created_by = <asserted author> and forked_from starting mcp:, and that row matches demoListQuery("mine", …) — the audit's G5-router variant;
  • 403 for someone else's demo, even with a valid secret;
  • 403 for a browser-made demo, with the pointer to /edit/:id;
  • 410 for a revoked demo (and no write, no artifact — gone, not rebuilt); 404 for an unknown id.

Nothing on the list had to be skipped.

Verification

  • pnpm test — 417/417 green (was 410); pnpm typecheck — clean.
  • Guard-bite check, both layers: re-inlining the forked_from check fails the source-grep test; neutering the guard behind false && (invisible to grep) fails the route-level 403 test. Both mutations reverted.
  • npx wrangler deploy --dry-run --containers-rollout=none in workers/api — passes. The bare --dry-run stops at the container image build because Docker isn't installed on this machine; identical error on untouched master, i.e. pre-existing and environmental, and the Worker bundle itself builds either way.

Note

Low Risk
Production change is a refactor of an existing MCP update guard into isMcpCreated() with stronger tests; no new auth surface or behavior change intended beyond testability.

Overview
Closes DEV-2501 audit gaps on MCP create/update by making the containment guard (forked_from must start with mcp:) a real, shared predicate and by exercising the actual API router in tests.

Security control: Adds exported isMcpCreated() in mcp-create.ts and switches PATCH /api/mcp/demos/:id to call it instead of an inline forked_from?.startsWith("mcp:"). Unit tests now import that function (not a local duplicate), plus a source-grep test ensures the route cannot drift back to inlined logic.

Route coverage: New mcp-routes.test.mjs drives workers/api/src/index.ts through worker.fetch with in-memory D1/KV/R2 fakes. Module hooks (worker-hooks.mjs + cloudflare-sandbox-stub.mjs) remap .js imports to .ts and stub @cloudflare/sandbox so Node tests never boot containers (create uses a fake build-cache hit).

Behaviors asserted: create without description → 400 with no D1/R2 writes; successful create → 201 response keys/URLs and created_by / forked_from provenance; update → 403 (wrong owner, browser-made demo), 410 (revoked, no rebuild), 404 (missing id).

Reviewed by Cursor Bugbot for commit a88d875. Bugbot is set up for automated code reviews on this repo. Configure here.

…DEV-2501)

The coverage audit's two worst findings, closed.

G1 — the containment guard was asserted against a copy of itself. The
"only demos the MCP created are updatable through it" test declared its
own local `fromMcp` predicate instead of importing the route's check, so
deleting the guard from index.ts left the test green — a test of a
security control that cannot fail when the control goes away. The
predicate now lives in mcp-create.ts as isMcpCreated() (the containment
control from the PR #177 security review), the route calls it, the test
imports it, and a source-grep test (theme-codegen.test.mjs style) pins
the route to the exported predicate so an inline check cannot drift
back in.

G2 — nothing imported the router, so every route-level promise of the
MCP endpoints (status codes, response shapes) was untested.
pipeline/mcp-routes.test.mjs now drives the real default export of
workers/api/src/index.ts under plain `node --test`, via module hooks
(fixtures/worker-hooks.mjs) that map the worker's ".js" specifiers onto
its ".ts" sources and stand a structural stub in for @cloudflare/sandbox
(its "cloudflare:" imports only exist inside workerd). Bindings are
in-memory fakes covering exactly what the two routes touch; the create
path takes createDemo()'s build-cache-hit branch, so no test ever boots
a container.

Covered end to end: the required-description 400 (with no D1 write), the
201 create shape (all four links + createdBy), the stored owner matching
the "My demos" listing query, both 403s (foreign owner; browser-made
demo), 410 for revoked, and 404 for unknown.

Verified both layers bite: re-inlining the forked_from check fails the
source-grep test, and neutering the guard behind `false &&` (invisible
to grep) fails the route-level 403 test.
Comment thread runner/pipeline/mcp-create.test.mjs
Dan Zyto added 2 commits August 18, 2026 17:39
…ute-coverage

# Conflicts:
#	runner/workers/api/src/index.ts
…ersion move)

The strict key-set assertion caught master adding htVersion to the
create response — exactly what it exists for. Admitted after review,
with a concreteness check: the agent pins its follow-up update to this
value, so a dist-tag here would be a bug.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit a88d875. Configure here.


register("./fixtures/worker-hooks.mjs", import.meta.url);

const { default: worker } = await import("../workers/api/src/index.ts");

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hooks registered without await

Medium Severity

module.register() is fired and the worker is imported on the next line without waiting for registration to finish. Hook setup is asynchronous, so the .js.ts remap and @cloudflare/sandbox stub can miss the load and fail intermittently.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit a88d875. Configure here.

@danielzytohoc
danielzytohoc requested a review from demtario August 18, 2026 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant